Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable mouse input when the debugger interaction tool is active #2209

Merged
merged 8 commits into from
Mar 10, 2019

Conversation

Dovyski
Copy link
Contributor

@Dovyski Dovyski commented Jan 22, 2019

FlxG.mouse.enabled will temporarily return false if the debugger interaction tool is active (visible). FlxButton (and any subclass) and FlxMouseEventManager will now account for FlxG.mouse.enabled when dealing with states, so clickable elements related to those will not work when the debugger interaction tool is active.

I decided to preserve the state of clickable elements when the debugger interaction tool becomes active, i.e. over remains over instead of released. I think it makes more sense to developers to not have a lot of state changes fired when the debugger is enabled. If you dislike this approach, let me know so I can adapt the code to no preserve the state of clickable elements.

Fixes: #2155

FlxG.mouse.enabled will temporarily return false if the debugger interaction tool is active (visible). FlxButton (and any subclass) and FlxMouseEventManager will now account for FlxG.mouse.enabled when dealing with states, so clickable elements related to those will not work when the debugger interaction tool is active.
@Gama11
Copy link
Member

Gama11 commented Jan 22, 2019

Hm, I think it would be a lot cleaner to en- and disable as needed, instead of FlxMouse.enabled having hardcoded knowledge about the internals of the debugger. FlxDebugger already follows that pattern in onMouseFocus() and onMouseFocusLost().

@Dovyski
Copy link
Contributor Author

Dovyski commented Jan 22, 2019

Agreed, that way is much better. In such case, the interaction tool should keep track of the current value of FlxG.mouse.enabled when the debugger (or the interaction itself) becomes active, so the value can be restored. Right?

@Gama11
Copy link
Member

Gama11 commented Jan 22, 2019

You mean in case the user has set the value? That's currently not done, nor is it done for FlxG.keys.enabled usages, so it might be a bit overkill.

@Dovyski
Copy link
Contributor Author

Dovyski commented Jan 22, 2019

Yeah, in case the user has set it. But since that is not the case, I'll just set true/false accordingly.

@@ -483,10 +483,15 @@ class FlxDebugger extends Sprite
@:allow(flixel.system.debug)
function onMouseFocusLost():Void
{
#if FLX_MOUSE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor complaint, but it looks we just need a single FLX_MOUSE check for this entire function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, no need for two checks.

@Gama11 Gama11 mentioned this pull request Jan 25, 2019
@Dovyski
Copy link
Contributor Author

Dovyski commented Feb 2, 2019

CI is failing in flixel.system.replay.FlxReplayTest#testButtonTrigger when the target is C++. It seems a button is not being clicked. I'll check on that.

@Gama11
Copy link
Member

Gama11 commented Feb 4, 2019

Oh, so the CI failure is legit for once. Usually it's just random failures...

@Dovyski
Copy link
Contributor Author

Dovyski commented Feb 4, 2019

They might be random =P. Build #5251.3 failed because of network issues (I think), but #5251.7 seems to be legit.

@Gama11
Copy link
Member

Gama11 commented Feb 27, 2019

Have you had a chance to look at the test failure yet? Updated the branch to latest dev, but it's still happening.

@Dovyski
Copy link
Contributor Author

Dovyski commented Feb 27, 2019

Not yet, I'm sorry. I can check on it this weekend.

@Dovyski
Copy link
Contributor Author

Dovyski commented Mar 4, 2019

It seems the cpp targets are still failing (now on a different test cases). When I run the tests locally (Win10, Haxe 3.4.7, Haxeflixel dev) they all pass though.

Am I missing anything?

@Gama11
Copy link
Member

Gama11 commented Mar 4, 2019

I do get those failures locally as well.

When FlxG.mouse.enabled is false, FlxMouseEventManager will continue to handle over and out events, but will ignore all other events. It allows tracked objects to be in a consistent state when mouse input is disabled, e.g. the interaction tool becomes active and buttons should stop working.
@Dovyski
Copy link
Contributor Author

Dovyski commented Mar 5, 2019

I think it is finally working.

#if FLX_MOUSE
// Debugger is visible, allow mouse input in the game only if the
// interaction tool is not active.
FlxG.mouse.enabled = !FlxG.game.debugger.interaction.visible;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit annoying - just because it's visible doesn't mean it's actively being used. I'd like to still be able to interact with buttons as long as none of the interaction tools are selected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I'll change that.

@Gama11
Copy link
Member

Gama11 commented Mar 10, 2019

Looks good to me now, thanks! 👍

@Gama11 Gama11 merged commit b6859af into HaxeFlixel:dev Mar 10, 2019
@Dovyski
Copy link
Contributor Author

Dovyski commented Mar 10, 2019

Thank you for all the feedback and for merging this PR! 🎉

@Gama11
Copy link
Member

Gama11 commented Mar 11, 2019

I even found two new issues while testing this PR, one fixed (605a18b) and one leftover for you (#2214). ;)

@Dovyski
Copy link
Contributor Author

Dovyski commented Mar 11, 2019

Nice catch! I'll get that leftover soon :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants